You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Actual fix for domain matching edge case related to substring listen paths. Fixed the sorting function for the listen paths and added some unit tests.
Extra: httpbin service not working on macos with apple silicon; now it does
Description
Related Issue
Motivation and Context
How This Has Been Tested
Screenshots (if appropriate)
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Refactoring or add test (improvements in base code or adds test coverage to functionality)
Checklist
I ensured that the documentation is up to date
I explained why this PR updates go.mod in detail with reasoning why it's required
I would like a code coverage CI quality gate exception and have explained why
PR Type
Bug fix, Configuration changes
Description
Fixed the domain sorting logic in the API loader by changing the comparison from domain length to lexicographical order to ensure correct domain matching.
Updated the Docker configuration for the httpbin service to specify the platform as linux/amd64, addressing compatibility issues on macOS with Apple Silicon.
Changes walkthrough 📝
Relevant files
Bug fix
api_loader.go
Fix domain sorting logic in API loader
gateway/api_loader.go
Modified sorting logic for API specs by domain.
Changed comparison from domain length to lexicographical order.
Here are some key observations to aid the review process:
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review
Domain Sorting Logic The change from length-based to lexicographical domain sorting may introduce issues with domain matching where subdomains are involved. This needs thorough testing to ensure no regression or unexpected behavior in domain matching.
Why: The suggestion to make domain comparison case-insensitive is valid and improves the robustness of the sorting logic by ensuring consistent behavior across different case variations.
7
Possible issue
Check compatibility of the specified platform with deployment environments
Verify the compatibility of the specified platform 'linux/amd64' with all potential deployment environments to ensure there are no runtime issues on different architectures.
-platform: linux/amd64+platform: linux/amd64 # Ensure compatibility with deployment environments
Suggestion importance[1-10]: 1
Why: The suggestion is a general reminder to check compatibility, which is a good practice but does not directly improve or modify the code. It's more of a cautionary note than a code improvement.
andrei-tyk
changed the title
[TT-13507] possible fix for domain matching and httpbin not working on…
[TT-13507] Fix for custom domains with substring listen path
Nov 18, 2024
The reason will be displayed to describe this comment to others. Learn more.
LGTM, this implements the agreed algo, where all paths are order descending by listen path, but those API that have custom domain will be placed in front of those without.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
andrei-tyk
changed the title
User description
Actual fix for domain matching edge case related to substring listen paths. Fixed the sorting function for the listen paths and added some unit tests.
Extra: httpbin service not working on macos with apple silicon; now it does
Description
Related Issue
Motivation and Context
How This Has Been Tested
Screenshots (if appropriate)
Types of changes
Checklist
PR Type
Bug fix, Configuration changes
Description
linux/amd64, addressing compatibility issues on macOS with Apple Silicon.Changes walkthrough 📝
api_loader.go
Fix domain sorting logic in API loadergateway/api_loader.go
httpbin.yml
Specify platform for httpbin service in Docker configurationdocker/services/httpbin.yml